-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load from cluster spec #218
Load from cluster spec #218
Conversation
Pull Request Test Coverage Report for Build 9c8b672d-cc85-4a3c-bef4-046d0f1245b7
💛 - Coveralls |
@helio-frota @alexalikiotis @danbev @tremes anything to comment here. I might merge |
lib/openshift-rest-client.js
Outdated
@@ -144,6 +151,9 @@ async function openshiftClient (settings = {}) { | |||
} | |||
|
|||
const client = new Client(clientConfig); | |||
if (settings.loadSpecFromCluster) { | |||
await client.loadSpec(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await
will throw if the promise is rejected. Maybe add try catch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea. i think that makes sense. Pretty sure that the underlying client fails silently and will always resolve. I will probably also do the same.
I do wonder if it makes sense to load the spec we include if this fails.
The reason for "failing" would be if there was no "/openapi/v2 " or "swagger.json" endpoints or not logged in and the spec couldn't be loaded. In that case there would be no api generated, i wonder if that is what we want. That could also be a different PR if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe load the included spec, and emit a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, thats what ii was thinking, but forgot to mention :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think my latest push resolves this
@@ -189,3 +190,25 @@ test('test different config - different location as a string', async (t) => { | |||
t.equal(kubeconfig.currentContext, 'for-node-client-testing/192-168-99-100:8443/developer', 'current context is correctly loaded'); | |||
t.end(); | |||
}); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for the failing condition perhaps?
LGTM |
62ef669
to
c96ca2f
Compare
This adds an option,
loadSpecFromCluster
, that when true, will make a call to/openapi/v2
orswagger.json
on your remote cluster which will be loaded in the client.By default, the client will load the spec file that we bundle, so there is no change for current users.
The main motivation for this, is if someone has a cluster were they have added some operators that add extensions, they would not be able to interact with those, since the spec file we load, only has the base kube and openshift api's. If they used this new option, then all the spec API's they have on their cluster will get loaded.
This type of thing is going to be needed for our nodeshift knative support.
Open Question: If both of those endpoints i named above fail to load, do we just load the bundled spec instead?